OpenVPN netifd Deep Integration & Configuration Refactoring#29220
OpenVPN netifd Deep Integration & Configuration Refactoring#29220ptpt52 wants to merge 3 commits intoopenwrt:masterfrom
Conversation
ptpt52
changed the title
ptpt52
force-pushed
the
pr-20260425
branch
3 times, most recently
from
bb192fb to
547196e
Compare
|
Hi, I tested this with a VPN profile and I think there are a couple of issues that should be considered. First, the netifd hotplug script is not guaranteed to be the script that actually runs. With profiles that already contain In my test the script that finally ran was the profile one:
So netifd never receives the That also means no routes are exported through netifd. I think The second issue is route handling. If I remove the custom So simply forcing the tunnel up is not enough. The pushed routes should be delegated to netifd in a way that preserves the WAN path to the VPN server and handles reconnect/cleanup safely. I also get this warning with the current behavior:
In my case the server already pushes Thanks edit for a better explanation |
|
@pesa1234 |
ptpt52
force-pushed
the
pr-20260425
branch
2 times, most recently
from
2c9be72 to
ebb8340
Compare
|
update: |
|
|
all route ok ? all setup looks good? |
Expected: Current result: So the route exists, but the From openvpn manual: |
|
All the options I added are designed to disable OpenVPN's internal network setup and fully delegate network and routing management to netifd. As a result, some user-provided networking options in the configuration file will be intentionally ignored or overridden. |
|
In OpenWrt, you can configure routing metrics to manage multiple default routes without conflicts. By adjusting the metric value, you can determine the priority of each default gateway. |
|
My opinion is that delegating network and route setup to netifd is the right direction, but the client should still preserve OpenVPN route semantics. In many cases the OpenVPN server is not managed by the OpenWrt user. For example, with commercial VPN providers or company VPNs, the client just receives whatever the server pushes. So I do not think OpenWrt should require the server-side config to be changed in order to avoid regressions. With For example, if the server pushes: then I would expect the OpenWrt client to preserve the normal OpenVPN behavior: while keeping the original WAN default route. If this is converted instead into: then the VPN still works, but the So I agree with netifd owning the route setup, but I think it should reproduce the routes OpenVPN would have installed, rather than filtering server-pushed default routes and injecting a different route layout. This is just my view and what I wanted to point out after testing. I am not trying to block a specific implementation direction. If the preferred approach is to make netifd fully authoritative and intentionally override some OpenVPN behavior, I can adapt to that direction. I just wanted to make sure the possible behavior change is explicit and understood before merging. |
ptpt52
force-pushed
the
pr-20260425
branch
3 times, most recently
from
8447dce to
9e04201
Compare
|
Thank you for the detailed feedback and for raising this concern. You are absolutely right that redirect-gateway def1 has specific semantics and generates two /1 routes to override the default route without touching the original 0.0.0.0/0. The reason I intentionally filtered out those /1 routes and replaced them with a standard 0.0.0.0/0 (with a specific metric) is that the def1 approach is essentially a legacy workaround. It was designed for OS environments that do not handle multiple default routes gracefully. In the OpenWrt ecosystem, however, netifd handles multiple 0.0.0.0/0 routes elegantly through metric priorities. Injecting 0.0.0.0/1 and 128.0.0.0/1 into the OpenWrt routing table often causes unintended conflicts with advanced networking packages like mwan3 (Multi-WAN) or Policy-Based Routing (PBR), which expect standard default routes to apply their rules. By mapping the VPN's intent (global redirect) into a standard OpenWrt default route with a higher priority (lower metric), we get a much cleaner integration with the firewall and native routing ecosystem, even if it changes the strict OpenVPN def1 behavior. Given this context, do you think we should document this as an intentional behavior change for better OpenWrt ecosystem compatibility? Or do you still strongly prefer simulating the exact /1 behavior via netifd to avoid any user surprise? I'm happy to adapt to either, but wanted to share the motivation behind this design. |
ptpt52
changed the title
|
updated |
This commit significantly improves the robustness of OpenVPN's netifd integration by enforcing strict control over routing, interface lifecycles, and configuration parsing. Key changes: - Makefile: Bump PKG_RELEASE to 3. - openvpn.sh: Force essential options (`persist-tun`, `persist-key`, `script-security`, and hotplug scripts) via command line arguments. - openvpn.sh: Actively filter out duplicate directives from user-provided configurations using `sed` to prevent OpenVPN warnings or conflicts (e.g., duplicated `up`/`down` scripts or `redirect-gateway`). - openvpn.sh: Auto-inject `--redirect-gateway def1 ipv6` for client configs based on the new `defaultroute` proto option. - openvpn.sh: Reorder `proto_openvpn_teardown` to kill the process before file deletion, and explicitly invoke the `cleanup` script. - openvpn-hotplug: Save `daemon_pid` and `ifname` into the interface's ubus data during the `up` phase for better state tracking. - openvpn-hotplug: Add `is_openvpn_default_route` helper to ignore server-pushed default routes, avoiding routing conflicts. - openvpn-hotplug: Dynamically calculate the route path to the VPN server endpoint and store the precise deletion commands in ubus, executing them reliably during the `cleanup` phase. - openvpn-hotplug: Redirect stderr to syslog for easier debugging. Signed-off-by: Chen Minqiang <ptpt52@gmail.com>
The previous json_add_int mtu call was outside proto_add_data block and had no effect. Replace it with an explicit ip link set on the tun device in the up handler before address configuration. Signed-off-by: Chen Minqiang <ptpt52@gmail.com>
- Introduce CONF_DIR/CONF_PREFIX constants for runtime file paths - Add rewrite_config_line helper to update `config` directives, handling absolute/relative paths and no/single/double quotes with optional trailing whitespace - Add copy_config_recursive to recursively copy all files referenced by `config` directives into CONF_DIR, rewriting paths in-place and guarding against circular includes - Apply all sed dedup operations across CONFIG_FILES so options in any included config file are also filtered - Use CONFIG_FILES glob for is_openvpn_client and redirect-gateway dedup to cover directives in included configs - Simplify teardown cleanup with glob pattern Signed-off-by: Chen Minqiang <ptpt52@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the OpenWrt netifd OpenVPN proto integration to “sandbox” user config files under /var/run, centralize routing/IP management under netifd (via --ifconfig-noexec/--route-noexec + hotplug), improve interface lifecycle behavior (e.g., MTU application), and expose more runtime state through ubus.
Changes:
- Add recursive config-copying + config-line rewriting into
/var/runand filter conflicting/duplicate OpenVPN options across all copied configs. - Deepen
netifdrouting control (default-route handling, filtering server-pushed default-like routes, storing cleanup route info in ubus). - Adjust teardown ordering and bump
PKG_RELEASE.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| net/openvpn/files/usr/libexec/openvpn-hotplug | Adds cleanup mode, default-route filtering, MTU application via ip link, and ubus state persistence for later cleanup. |
| net/openvpn/files/lib/netifd/proto/openvpn.sh | Implements config sandboxing + option filtering, forces hotplug/noexec/persist behavior, adds defaultroute proto option, and changes teardown cleanup flow. |
| net/openvpn/Makefile | Bumps PKG_RELEASE from 2 to 3. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| append exec_params "--redirect-gateway def1 ipv6" | ||
| [ -n "$defaultroute" ] || defaultroute=1 | ||
| sed -i '/^[[:space:]]*redirect-gateway[[:space:]]*/s/^/# /' $CONFIG_FILES |
There was a problem hiding this comment.
The new defaultroute proto option is not currently respected: when is_openvpn_client is true, the code always appends --redirect-gateway def1 ipv6 and comments out any existing redirect-gateway lines, even if defaultroute=0 was configured. Gate the redirect-gateway injection (and the sed filtering) on defaultroute being enabled, and only default it to 1 when unset.
| append exec_params "--redirect-gateway def1 ipv6" | |
| [ -n "$defaultroute" ] || defaultroute=1 | |
| sed -i '/^[[:space:]]*redirect-gateway[[:space:]]*/s/^/# /' $CONFIG_FILES | |
| [ -n "$defaultroute" ] || defaultroute=1 | |
| if [ "$defaultroute" != "0" ]; then | |
| append exec_params "--redirect-gateway def1 ipv6" | |
| sed -i '/^[[:space:]]*redirect-gateway[[:space:]]*/s/^/# /' $CONFIG_FILES | |
| fi |
| exit | ||
| } | ||
|
|
||
| exec 2> >(logger -t "openvpn_$INTERFACE(hotplug)" -p daemon.err) |
There was a problem hiding this comment.
exec 2> >(logger ...) uses process substitution, which is not supported by POSIX /bin/sh (and typically not available in BusyBox ash used by OpenWrt). This will cause the hotplug script to fail to start. Redirect stderr in a POSIX-compatible way (e.g., via a pipe/FD redirection pattern that works in sh, or explicitly run under a shell that supports process substitution).
| exec 2> >(logger -t "openvpn_$INTERFACE(hotplug)" -p daemon.err) | |
| logpipe="/var/run/openvpn-hotplug-$INTERFACE.$$" | |
| rm -f "$logpipe" | |
| mkfifo "$logpipe" || { | |
| logger -t "openvpn(proto)" -p daemon.warn "hotplug: failed to create log pipe '$logpipe'" | |
| exit 1 | |
| } | |
| logger -t "openvpn_$INTERFACE(hotplug)" -p daemon.err <"$logpipe" & | |
| logpipe_pid=$! | |
| exec 2>"$logpipe" | |
| trap 'rm -f "$logpipe"; wait "$logpipe_pid" 2>/dev/null' EXIT |
| json_data="$(ubus call network.interface."$INTERFACE" status 2>/dev/null)" | ||
| if json_load "$json_data" 2>/dev/null; then | ||
| if json_select data 2>/dev/null; then | ||
| json_get_var SERVER_ROUTE_IPV4_CLEANUP_CMD server_route_ipv4_cleanup_cmd | ||
| json_get_var SERVER_ROUTE_IPV6_CLEANUP_CMD server_route_ipv6_cleanup_cmd | ||
| [ -n "$SERVER_ROUTE_IPV4_CLEANUP_CMD" ] && $SERVER_ROUTE_IPV4_CLEANUP_CMD | ||
| [ -n "$SERVER_ROUTE_IPV6_CLEANUP_CMD" ] && $SERVER_ROUTE_IPV6_CLEANUP_CMD | ||
| fi |
There was a problem hiding this comment.
The cleanup phase executes a command string loaded from ubus ($SERVER_ROUTE_IPV*_CLEANUP_CMD). This is brittle and expands as shell code (word-splitting/globbing), which can become a command-injection vector if the stored string is ever influenced unexpectedly. Store structured route fields (family/target/gw/dev) and reconstruct the ip route del invocation with proper quoting instead of executing an arbitrary string.
| rewrite_config_line() { | ||
| local dst="$1" | ||
| local old="$2" | ||
| local new="$3" | ||
| local fname="${old##*/}" | ||
|
|
||
| # absolute path, three quote styles | ||
| sed -i "s|^\([[:space:]]*config[[:space:]]\+\)${old}[[:space:]]*$|\1${new}|" "$dst" | ||
| sed -i "s|^\([[:space:]]*config[[:space:]]\+\)'${old}'[[:space:]]*$|\1${new}|" "$dst" | ||
| sed -i "s|^\([[:space:]]*config[[:space:]]\+\)\"${old}\"[[:space:]]*$|\1${new}|" "$dst" | ||
| # relative filename, three quote styles | ||
| sed -i "s|^\([[:space:]]*config[[:space:]]\+\)${fname}[[:space:]]*$|\1${new}|" "$dst" | ||
| sed -i "s|^\([[:space:]]*config[[:space:]]\+\)'${fname}'[[:space:]]*$|\1${new}|" "$dst" | ||
| sed -i "s|^\([[:space:]]*config[[:space:]]\+\)\"${fname}\"[[:space:]]*$|\1${new}|" "$dst" |
There was a problem hiding this comment.
rewrite_config_line interpolates $old directly into a sed regex. If the source path contains regex metacharacters (e.g., ., [, ], *, \, |, &), the substitution can match incorrectly or fail. Escape the path for use in sed (both the match and replacement side) before running the replacements.
| # expand relative path to absolute using cd_dir | ||
| case "$ref" in | ||
| /*) ;; | ||
| *) ref="$cd_dir/$ref" ;; | ||
| esac | ||
|
|
||
| # cycle guard | ||
| case "$visited" in | ||
| *"|$ref|"*) continue ;; | ||
| esac | ||
|
|
||
| [ -f "$ref" ] || continue | ||
|
|
||
| fname="${ref##*/}" | ||
| dst_ref="${CONF_PREFIX}${config}.user_${fname}" | ||
|
|
||
| cp "$ref" "$dst_ref" || { | ||
| logger -t "openvpn_$config(proto)" -p daemon.err "failed to copy config '$ref' to '$dst_ref'" | ||
| continue | ||
| } | ||
|
|
||
| # rewrite the `config` line in the parent file to point to the copy | ||
| rewrite_config_line "$dst" "$ref" "$dst_ref" | ||
|
|
There was a problem hiding this comment.
copy_config_recursive converts relative config references to absolute paths (ref="$cd_dir/$ref") and then calls rewrite_config_line using that absolute path. If the original config line was a relative path with directories (e.g., config subdir/extra.conf), rewrite_config_line won't match (it only handles absolute paths or a bare filename), so the parent file may continue referencing the original user file instead of the sandbox copy. Preserve the original token (as written) and rewrite using that, or enhance rewrite_config_line to handle relative paths containing / components.
| [ -f "$ref" ] || continue | ||
|
|
||
| fname="${ref##*/}" | ||
| dst_ref="${CONF_PREFIX}${config}.user_${fname}" |
There was a problem hiding this comment.
Sandbox copy names are based only on the included file basename (dst_ref="...user_${fname}"). If two different includes share the same basename (e.g., a/ca.conf and b/ca.conf), the later copy overwrites the first, and the rewritten config lines will both point at the same file. Use a collision-resistant destination name (e.g., include a hash of the full source path or mirror the directory structure under ${CONF_DIR}) to avoid accidental overwrites.
| dst_ref="${CONF_PREFIX}${config}.user_${fname}" | |
| ref_hash="$(printf '%s' "$ref" | md5sum | cut -d' ' -f1)" | |
| dst_ref="${CONF_PREFIX}${config}.user_${ref_hash}_${fname}" |
I am with @pesa1234 I would prefer the standard OpenVPN behaviour with /1. But thanks for your great work especially happy with :
|
OpenVPN Proto Handler Redesign: Centralized Route Management via netifdProblem StatementThe original OpenVPN proto handler delegates route and interface management directly to OpenVPN, which has several limitations in modern OpenWrt deployments: 1. Route Management ConflictsOpenVPN's In early Linux kernels, the kernel could only maintain a single default route. OpenVPN's Modern Linux kernels support multiple routes with different metrics—a far more elegant solution. However, 2. Poor Integration with Multi-WANThe hardcoded route injection prevents seamless cooperation with
3. Scattered ConfigurationUser config files scattered across the filesystem make debugging difficult and leave room for path resolution errors: When paths fail to resolve or certificates are moved, troubleshooting requires searching multiple directories. 4. Limited Interface State Trackingnetifd cannot properly track or manage routes that OpenVPN injects independently, breaking the unified interface management model. This causes issues with:
SolutionThis redesign centralizes all IP and route management under netifd's control: 1. Unified Config ManagementAll user config files are copied to Benefits:
2. Route Management via netifdRemove OpenVPN's built-in route injection and let netifd handle all routing: Route Handling:
3. Hotplug Script IntegrationAlways apply script hooks via netifd context, not standalone OpenVPN execution: Benefits:
Technical ArchitectureConfig File Processing# 1. Copy user config
cp /etc/openvpn/client/client.conf → /var/run/openvpn.client.user.conf
# 2. Scan for `config` directives
grep "^[[:space:]]*config" /var/run/openvpn.client.user.conf
→ /etc/openvpn/client/extra.conf
# 3. Recursively copy referenced files
cp /etc/openvpn/client/extra.conf → /var/run/openvpn.client.user_extra.conf
# 4. Rewrite paths in copied file
sed -i 's|config /etc/openvpn/client/extra.conf|config /var/run/openvpn.client.user_extra.conf|' \
/var/run/openvpn.client.user.conf
# 5. Repeat for nested includes (cycle guard prevents infinite loops)Route Injection FlowCycle Guard for Nested Includes# Track visited files to prevent circular references
copy_config_recursive() {
local dst="$1"
local visited="$2" # Format: "|/path1|/path2|..."
grep "^[[:space:]]*config" "$dst" | while read ref; do
# Check if already processed
case "$visited" in
*"|$ref|"*) continue ;; # Skip circular reference
esac
# Process and recurse
copy_config_recursive "$dst_ref" "$visited|$ref|"
done
}Benefits✓ Better Multi-WAN SupportSeamless integration with ✓ Firewall CompatibilityRoutes respect firewall rules and policies. Interface zone assignments properly apply to VPN traffic without special handling. ✓ Cleaner DebuggingAll config files in one place ( ✓ Proper CleanupHotplug script ensures all routes cleaned on disconnect. netifd's automatic cleanup prevents orphaned routes. ✓ Future-ProofAligns with modern Linux kernel capabilities (multi-default routes with proper metrics). Not tied to legacy workarounds. ✓ Unified ManagementSingle point of control via netifd interface config. No separate OpenVPN route management required. ✓ Reduced Configuration ErrorsPath resolution happens at setup time with clear error messages, not at runtime with cryptic OpenVPN errors. Backward CompatibilityUser-Facing ChangesNone. User OpenVPN config files remain completely unchanged: Internal Changes Only
Migration PathNo migration needed. Existing deployments will automatically benefit from the redesign with no configuration changes. Testing Recommendations1. Client Mode
2. Server Mode
3. Failover Scenarios
4. Configuration Testing
5. Cleanup & State Management
6. Error Handling
Migration GuideFor End UsersNo action required. The change is transparent at the UCI config level. Existing configurations continue to work exactly as before. For System AdministratorsDebugging is now easier: # Old way: trace paths across filesystem
ls /etc/openvpn/client/
ls /etc/openvpn/keys/
# New way: inspect actual files being used
ls /var/run/openvpn.vpn_client.*
cat /var/run/openvpn.vpn_client.confFor DevelopersThe proto handler now provides better integration points: # Monitor route changes
ubus call network.interface.vpn status | jq .route
# Check interface state
ubus call network.interface.vpn status | jq .up
# Integrate with custom scripts
/etc/hotplug.d/openvpn/99-custom-handlerImplementation DetailsNew Functions
|
🚀 OpenVPN netifd Deep Integration & Configuration Refactoring
📑 Overview
This update introduces a series of major refactoring and enhancements to the OpenVPN module within the OpenWrt/netifd environment. The core objectives are to improve the robustness of network and routing management, implement sandbox protection for user configurations, and fix known defects in the interface lifecycle (e.g., ineffective MTU settings).
By forcibly taking over underlying network configuration and introducing a smarter configuration parser, this commit thoroughly resolves conflicts between OpenVPN's built-in routing mechanisms and OpenWrt's system-level network manager (
netifd).✨ Key Improvements & Features
1. Configuration "Sandboxing" & Safe Parsing
configdirective into the/var/runramdisk (CONF_DIR) for centralized, safe management.copy_config_recursiveandrewrite_config_linehelper functions. These safely handle nestedconfigdirectives, automatically resolve absolute/relative paths, manage quotes and trailing whitespace, and incorporate a strict circular include prevention mechanism.sedfiltering of conflicting options (e.g.,up/down,script-security) now spans across all associated configuration files (theCONFIG_FILESglob). This completely eliminates duplicate parameter warnings during OpenVPN startup.2. Routing Control & Deep netifd Integration
--ifconfig-noexec,--route-noexec, and thehotplugscripts via command-line arguments. This delegates all IP and routing states entirely tonetifd, ensuring the system's native routing table and Multi-WAN (mwan3) strategies are respected.is_openvpn_default_routehelper in thehotplugscript to actively ignore global default routes pushed by the VPN server (e.g.,0.0.0.0/1). This prevents server configurations from breaking the client's local routing policies.defaultrouteproto option. For client mode configurations, the script automatically injects--redirect-gateway def1 ipv6.ubus. This prevents routing loops caused by default gateway shifts.3. Interface Lifecycle & State Tracking
json_add_int mtucall. Replaced it with an explicitip link set dev <tun> mtu <val>executed during theupphase (prior to address configuration), ensuring the MTU is correctly applied at the kernel level.daemon_pidand the actual device name (ifname) into the interface'subusdata during theupphase, making it easier for external tools to track OpenVPN states.proto_kill_commandbefore file cleanup begins, preventing read/write errors during teardown.cleanupphase of thehotplugscript to execute precise route removal.rm -f ${CONF_PREFIX}$iface.*) to clean up all related temporary configuration files in a single step.4. Additional Stability Enhancements
persist-tunandpersist-key. This ensures the virtual interface is not destroyed during reconnections and prevents permission errors when OpenVPN attempts to re-read certificates after dropping privileges.stderr) ofopenvpn-hotplugtosyslog(tagged with the interface name), significantly lowering the difficulty of troubleshooting network script failures.PKG_RELEASEto 3 in the Makefile.